-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Flatten migrations #1187
Flatten migrations #1187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a terrific change! Left some minor comments. The main concern I would have is with continuing the migration after a failed backup dump.
server/migrations/utils/index.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reorganization is welcome! I am going to assume no functional changes. Let me know if this assumption is incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to assume no functional changes
That's right!
const dumpTable = async tableName => { | ||
try { | ||
const dumpFilePath = path.resolve( | ||
`${__dirname}/../dumps/dumpTable_${tableName}_${new Date().getTime()}.sql` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These files may eventually crowd directories on our local machines. It is easy enough to delete but just pointing this out. I don't think there is an action item here unless you want to add something to purge especially old ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd rather this be left to each contributor to decide how to handle for themselves since they're gitignored. I suppose utils for clean:migrationDump:tables
or clean:migrationDump:databases
could be added but I don't think it's super necessary (to me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! No need for a util for now.
await exec( | ||
`pg_dump -t '"${tableName}"' ${process.env.PGDATABASE} --inserts > ${dumpFilePath}` | ||
); | ||
} catch (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to catch this? Might be safer to not run the migration if making the dump fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Addressed in 72861c2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for addressing the comments.
The migrations used in this project have grown as much as "deemed necessary" over the project's lifespan. This PR aims to remove those migrations and preserve the structure of the tables. Due to the suggested point for this PR's merge, there shouldn't be a need to preserve any functional changes of those previous migrations as all environments should be up to date migration-wise and the migrations were mostly focused on keeping up to date with changes to the application's code (from what I've gone through at least).
This should present a fresh start on the migrations by limiting the time taken to run them and also attempt to maintain the practice of, as best as possible to avoid using application code in any migration going forward.